Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[v8.x backport] 16238 and 18576 #20797

Closed
wants to merge 2 commits into from
Closed

[v8.x backport] 16238 and 18576 #20797

wants to merge 2 commits into from

Conversation

yhwang
Copy link
Member

@yhwang yhwang commented May 17, 2018 •
edited
Loading

Backport #16238 and #18576 to resolve issue #20763

fix: #20763

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8.x labels May 17, 2018
@yhwang
Copy link
Member Author

yhwang commented May 17, 2018

Let me run a CI to verify it and wait for the review. I don't know if it's okay to backport 2 PRs as one PR. If not I will create separated PRs for both.

CI: https://ci.nodejs.org/job/node-test-commit/18549/

@MylesBorins
Copy link
Contributor

@yhwang totally fine to backport multiple PRs

do you think this breakage should cause another patch release for next week or could it wait for the next scheduled relase eta 1 month

@yhwang
Copy link
Member Author

yhwang commented May 17, 2018

@MylesBorins actually the --enable-static could be deprecated after 658dd409fd. The reason being is static library is built when doing ./configure. Therefore, there is no need to use --enable-static. I think it's okay for next scheduled release.

@MylesBorins
Copy link
Contributor

MylesBorins commented May 22, 2018 •
edited
Loading

After landing I'm getting some errors

gyp: Undefined variable OBJ_SUFFIX in /Users/mborins/code/node/v8.x/node.gyp while trying to load /Users/mborins/code/node/v8.x/node.gyp
Error running GYP

Could we get a rebase and manual test to ensure this works

@yhwang
Copy link
Member Author

yhwang commented May 22, 2018 •
edited
Loading

@MylesBorins I thought CI did the job you mentioned. I can do rebase and verify it on ubuntu and windows manually.

[update] I know why you have the error above. because of this change: b2b85e5#diff-259e627be3fafed9e9c10a7fcb26879e

It added a section of new code into node.gyp and use the upper case variables. This new section should be removed. I can remove it in this PR

@danbev @yhwang
build: make gyp user defined variables lowercase
b01a26a 
I mistakenly introduced user defined variables using uppercase
characters, reading the gyp documentation they state:
"Predefined variables. By convention, these are named with
CAPITAL_LETTERS. Predefined variables are set automatically by GYP"
and also "By convention, user-defined variables are named with
lowercase_letters."

This commit renames the user defined variables to lowercase to follow
the above mentioned convention.

PR_URL: nodejs#20797
Original PR-URL: nodejs#16238
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

@yhwang if you could remove it in this PR that would be rad. Thanks

@danbev @yhwang
build: add node_lib_target_name to cctest deps
4aaa601 
Currently the cctest target depend on the node_core_target_name
target. But it is the node_lib_target_name target that compiles the
sources now which means that if a source file in src is updated the
cctest executable will not be re-linked against it, but will remain
unchanged. The code will still be compiled, just not linked which
means that if you are debugging you'll not see the changes and also a
warning will be displayed about this issue.

This commit changes the cctest target to depend on node_lib_target_name.
PR-URL: nodejs#20797
Original PR-URL: nodejs#18576
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Yihong Wang <yh.wang@ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@yhwang yhwang force-pushed the backport-16238-18576-to-v8.x branch from f0baab1 to 4aaa601 Compare May 22, 2018 20:22
@yhwang
Copy link
Member Author

yhwang commented May 22, 2018

CI: https://ci.nodejs.org/job/node-test-commit/18688/

@yhwang
Copy link
Member Author

yhwang commented May 22, 2018

parallel/test-timers-throw-reschedule this test case failed in other v8.x-staging builds. It's not related to this PR.

MylesBorins pushed a commit that referenced this pull request May 23, 2018
@danbev @MylesBorins
build: make gyp user defined variables lowercase
396c96e 
I mistakenly introduced user defined variables using uppercase
characters, reading the gyp documentation they state:
"Predefined variables. By convention, these are named with
CAPITAL_LETTERS. Predefined variables are set automatically by GYP"
and also "By convention, user-defined variables are named with
lowercase_letters."

This commit renames the user defined variables to lowercase to follow
the above mentioned convention.

Backport-PR-URL: #20797
PR-URL: #16238
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 23, 2018
@danbev @MylesBorins
build: add node_lib_target_name to cctest deps
04c166a 
Currently the cctest target depend on the node_core_target_name
target. But it is the node_lib_target_name target that compiles the
sources now which means that if a source file in src is updated the
cctest executable will not be re-linked against it, but will remain
unchanged. The code will still be compiled, just not linked which
means that if you are debugging you'll not see the changes and also a
warning will be displayed about this issue.

This commit changes the cctest target to depend on node_lib_target_name.

Backport-PR-URL: #20797
PR-URL: #18576
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Yihong Wang <yh.wang@ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins
Copy link
Contributor

landed in 0082efd...04c166a

@yhwang yhwang deleted the backport-16238-18576-to-v8.x branch May 23, 2018 16:42
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
@danbev @MylesBorins
build: make gyp user defined variables lowercase
8c3be5c 
I mistakenly introduced user defined variables using uppercase
characters, reading the gyp documentation they state:
"Predefined variables. By convention, these are named with
CAPITAL_LETTERS. Predefined variables are set automatically by GYP"
and also "By convention, user-defined variables are named with
lowercase_letters."

This commit renames the user defined variables to lowercase to follow
the above mentioned convention.

Backport-PR-URL: #20797
PR-URL: #16238
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
@danbev @MylesBorins
build: add node_lib_target_name to cctest deps
db17621 
Currently the cctest target depend on the node_core_target_name
target. But it is the node_lib_target_name target that compiles the
sources now which means that if a source file in src is updated the
cctest executable will not be re-linked against it, but will remain
unchanged. The code will still be compiled, just not linked which
means that if you are debugging you'll not see the changes and also a
warning will be displayed about this issue.

This commit changes the cctest target to depend on node_lib_target_name.

Backport-PR-URL: #20797
PR-URL: #18576
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Yihong Wang <yh.wang@ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@danbev @rvagg
build: make gyp user defined variables lowercase
9bd5fc2 
I mistakenly introduced user defined variables using uppercase
characters, reading the gyp documentation they state:
"Predefined variables. By convention, these are named with
CAPITAL_LETTERS. Predefined variables are set automatically by GYP"
and also "By convention, user-defined variables are named with
lowercase_letters."

This commit renames the user defined variables to lowercase to follow
the above mentioned convention.

Backport-PR-URL: #20797
PR-URL: #16238
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
@danbev @rvagg
build: add node_lib_target_name to cctest deps
18fd620 
Currently the cctest target depend on the node_core_target_name
target. But it is the node_lib_target_name target that compiles the
sources now which means that if a source file in src is updated the
cctest executable will not be re-linked against it, but will remain
unchanged. The code will still be compiled, just not linked which
means that if you are debugging you'll not see the changes and also a
warning will be displayed about this issue.

This commit changes the cctest target to depend on node_lib_target_name.

Backport-PR-URL: #20797
PR-URL: #18576
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Yihong Wang <yh.wang@ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yhwang @MylesBorins @nodejs-github-bot @danbev